Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: helm deploys with a password #9113

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

jesse-amano-hpe
Copy link
Contributor

@jesse-amano-hpe jesse-amano-hpe commented Apr 5, 2024

Description

DET-10208 Helm deploys with the initialUserPassword or defaultPassword

Test Plan

Helm deployment succeeds with initialUserPassword uncommented and set in values.yaml

Commentary

#9074 ended up taking care of a lot of the work needed for this ticket, this mostly documents the behavior in helm and makes it so we never launch a sidecar with defaultPassword (which, if initialUserPassword is also set, would error out anyway because its assumption that it can use an empty admin password would be incorrect).

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

DET-10208

@cla-bot cla-bot bot added the cla-signed label Apr 5, 2024
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Apr 5, 2024
@determined-ci determined-ci requested a review from a team April 5, 2024 05:50
Copy link

netlify bot commented Apr 5, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit bbc5034
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66100cf242bea80008515691

@jesse-amano-hpe jesse-amano-hpe force-pushed the jta/DET-10208/helm-deploy-with-password branch from 0098fd0 to 9034665 Compare April 5, 2024 05:54
@jesse-amano-hpe jesse-amano-hpe requested review from a team and maxrussell and removed request for a team April 5, 2024 05:56
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.64%. Comparing base (2ef5ab9) to head (bbc5034).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9113   +/-   ##
=======================================
  Coverage   46.63%   46.64%           
=======================================
  Files        1172     1172           
  Lines      143619   143619           
  Branches     2410     2410           
=======================================
+ Hits        66983    66986    +3     
+ Misses      76431    76428    -3     
  Partials      205      205           
Flag Coverage Δ
backend 43.39% <ø> (+<0.01%) ⬆️
harness 63.99% <ø> (ø)
web 37.77% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

@jesse-amano-hpe jesse-amano-hpe enabled auto-merge (squash) April 5, 2024 06:26
- ``defaultPassword``: Specifies a string containing the default password for the admin and
determined user accounts.
determined user accounts. (Deprecated; prefer ``initialUserPassword``)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean the "defaultPassword" setting should not be used? i.e.,

  • initialUserPassword: Specifies a string containing the default password for the admin and
    determined user accounts.

  • defaultPassword: (Deprecated) Specifies a string containing the default password for the admin and
    determined user accounts. Use initialUserPassword instead.

- Helm: When deploying a new cluster with Helm, an initial password for the ``admin`` and
``determined`` users should be specified using either ``initialUserPassword`` or
``defaultPassword`` (see helm/charts/determined/values.yaml). Configuring a password is no
longer done as a separate step.
Copy link
Contributor

@tara-det-ai tara-det-ai Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:orphan:

Security

  • Helm: When deploying a new cluster with Helm, configuring an initial password for the "admin" and "determined" users is required and is no longer a separate step. To specify an initial password for these users, visit the helm/charts/determined/values.yaml file and use either initialUserPassword (preferred) or
    defaultPassword (deprecated). For reference, visit :ref:helm-config-reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -57,7 +57,7 @@ stringData:

security:
{{- if .Values.initialUserPassword }}
initial_user_password: {{ .Values.initialUserPassword | quote }}
initial_user_password: {{ coalesce .Values.initialUserPassword .Values.defaultPassword | quote }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anything should change there -- that's from #8851 which describes the structure that this configuration file is using. In Helm's case, there was previous support for a .Values.defaultPassword that would launch a sidecar. Before that, the flow for configuring the admin password was something like:

  1. Launch a cluster with empty admin password
  2. Wait a bit for the helm postInstall hook to fire
  3. If defaultPassword is set,
    a. authorize as admin using empty password
    b. change the admin and determined passwords to .defaultPassword

After #8851 and #9074 , the flow is:

  1. Launch a cluster; if .Values.initialUserPassword is set, use that for admin/determined users, otherwise allow empty passwords
  2. Wait a bit for the helm postInstall hook to fire
  3. If defaultPassword is set,
    a. authorize as admin using no password (fails if .Values.initialUserPassword was also set, since admin has a non-empty password!)
    b. try to change the admin and determined passwords to .defaultPassword

By changing this line and deleting the sidecar, the intention (which I've manually tested, but probably not exhaustively!) is that the new flow is:

  1. Launch a cluster; if .Values.initialUserPassword is set, use that for admin/determined users; otherwise use .Values.defaultPassword; if both are blank, an attempt is made with a blank password (which will eventually fail because of all the other changes we're making around this).

I hope this is helpful/informative and not just over-explaining things you already knew! If there's something about this that needs to be communicated and isn't already handled in your #9101 I could use some help writing copy in the style most consistent with Determined docs. 😄

Copy link
Contributor

@tara-det-ai tara-det-ai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left comments and suggestions

@determined-ci determined-ci requested a review from a team April 5, 2024 14:38
@jesse-amano-hpe jesse-amano-hpe merged commit d5f807d into main Apr 8, 2024
73 of 85 checks passed
@jesse-amano-hpe jesse-amano-hpe deleted the jta/DET-10208/helm-deploy-with-password branch April 8, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation User-facing API Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants